-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(starknet_consensus_orchestrator): add sierras to cende blob #3955
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 7 files reviewed, 3 unresolved discussions (waiting on @dafnamatsry and @DvirYo-starkware)
crates/starknet_consensus_orchestrator/src/cende/central_objects_test.rs
line 601 at r1 (raw file):
#[tokio::test] async fn test_get_sierras_and_casms() {
note: the test is not operational yet, it is just the skeleton,
will be enabled in the next PRs, once I have the transactions testing instances done.
crates/starknet_consensus_orchestrator/src/cende/mod.rs
line 254 at r1 (raw file):
) -> CendeAmbassadorResult<()> { // TODO(dvir): as optimization, call the `into` and other preperation when writing to AS. - // Dvir, what is this? is it still relevant?
Dvir, question for you. is this still relevant?
Code quote:
// TODO(dvir): as optimization, call the `into` and other preperation when writing to AS. -
// Dvir, what is this? is it still relevant?
crates/starknet_consensus_orchestrator/src/cende/mod.rs
line 331 at r1 (raw file):
// TODO(Yael): remove the executable transactions and convert the rpc_transactions directly to // the central format. pub(crate) transactions: Vec<Transaction>,
this vector is here temporarily, will be deleted in the following PRs.
Code quote:
pub(crate) transactions: Vec<Transaction>,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 7 files reviewed, 5 unresolved discussions (waiting on @dafnamatsry and @DvirYo-starkware)
crates/starknet_consensus_orchestrator/src/cende/mod.rs
line 255 at r1 (raw file):
// TODO(dvir): as optimization, call the `into` and other preperation when writing to AS. - // Dvir, what is this? is it still relevant? let block_number = blob_parameters.block_info.block_number;
This is the same logic we previously had in the From function,
The only logic that has changed is how I get the contract classes.
crates/starknet_consensus_orchestrator/src/cende/mod.rs
line 243 at r1 (raw file):
} impl From<BlobParameters> for AerospikeBlob {
Since the new from logic requires self to be present, I moved it all to "prepare_blob_for_next_height",
The only logic that has changed is how I get the contract classes.
Code quote:
impl From<BlobParameters> for AerospikeBlob {
45c1cc9
to
4a971cc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 9 files reviewed, 3 unresolved discussions
crates/starknet_consensus_orchestrator/src/cende/mod.rs
line 255 at r1 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
This is the same logic we previously had in the From function,
The only logic that has changed is how I get the contract classes.
the contract classes and the transactions
4a971cc
to
ccd6109
Compare
ccd6109
to
96b113d
Compare
Artifacts upload workflows: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 7 files at r1, 8 of 9 files at r2, all commit messages.
Reviewable status: 9 of 10 files reviewed, 10 unresolved discussions (waiting on @dafnamatsry and @Yael-Starkware)
crates/starknet_consensus_orchestrator/src/cende/mod.rs
line 254 at r1 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
Dvir, question for you. is this still relevant?
Yes. We will not do it for now, but it is a possible optimization. Remove your comment // Dvir, what is this? is it still relevant?
crates/starknet_consensus_orchestrator/src/cende/central_objects.rs
line 322 at r2 (raw file):
sierra_version: into_string_tuple(SierraVersion::from_str( &sierra.contract_class_version, )?),
Please compare this with a real class.
Code quote:
sierra_program_size: sierra.sierra_program.len(),
abi_size: sierra.abi.len(),
sierra_version: into_string_tuple(SierraVersion::from_str(
&sierra.contract_class_version,
)?),
crates/starknet_consensus_orchestrator/src/cende/central_objects.rs
line 370 at r2 (raw file):
L1Handler(CentralL1HandlerTransaction), }
document that the Option<&SierraContractClass>
is only for declare
crates/starknet_consensus_orchestrator/src/cende/central_objects.rs
line 492 at r2 (raw file):
} }
I don't like the way it is done. Can we do something like function get_innder_declrae
and only if it return Some
does all the things of declare?
crates/starknet_consensus_orchestrator/resources/central_sierra_contract_class.json
line 6 at r2 (raw file):
"0x1", "0x2" ],
special reason for the change here?
crates/starknet_consensus_orchestrator/src/cende/mod.rs
line 63 at r2 (raw file):
execution_infos: Vec<CentralTransactionExecutionInfo>, contract_classes: Vec<CentralSierraContractClassEntry>, compiled_classes: Vec<CentralCasmContractClassEntry>,
Consider using raw tuple here
Code quote:
contract_classes: Vec<CentralSierraContractClassEntry>,
compiled_classes: Vec<CentralCasmContractClassEntry>,
crates/starknet_consensus_orchestrator/resources/central_declare_tx.json
line 30 at r2 (raw file):
"sierra_program_size": 3, "sierra_version": [ "0x0",
What is the reason for the change? Did you use real tx json?
crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs
line 439 at r2 (raw file):
}) .await // TODO(shahak): Do not panic here.
remove this
Code quote:
// TODO(shahak): Do not panic here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 9 of 10 files reviewed, 10 unresolved discussions (waiting on @dafnamatsry and @DvirYo-starkware)
crates/starknet_consensus_orchestrator/resources/central_declare_tx.json
line 30 at r2 (raw file):
Previously, DvirYo-starkware wrote…
What is the reason for the change? Did you use real tx json?
no reason
crates/starknet_consensus_orchestrator/resources/central_sierra_contract_class.json
line 6 at r2 (raw file):
Previously, DvirYo-starkware wrote…
special reason for the change here?
yes , it needs to be formatted to a three numbers string ,
in the declare tx
crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs
line 439 at r2 (raw file):
Previously, DvirYo-starkware wrote…
remove this
as discussed with matan : need to print an error and proceed normally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 9 of 10 files reviewed, 10 unresolved discussions (waiting on @dafnamatsry and @DvirYo-starkware)
crates/starknet_consensus_orchestrator/src/cende/central_objects.rs
line 492 at r2 (raw file):
Previously, DvirYo-starkware wrote…
I don't like the way it is done. Can we do something like function
get_innder_declrae
and only if it returnSome
does all the things of declare?
as discussed f2f, from process_transactions, I will call a different function for declare and a different function for the rest , and by that eliminate returning none values.
96b113d
to
616f6d4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 6 of 10 files reviewed, 9 unresolved discussions (waiting on @dafnamatsry and @DvirYo-starkware)
crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs
line 439 at r2 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
as discussed with matan : need to print an error and proceed normally.
done.
crates/starknet_consensus_orchestrator/src/cende/central_objects.rs
line 322 at r2 (raw file):
Previously, DvirYo-starkware wrote…
Please compare this with a real class.
code copied form here:
let class_info = ClassInfo { |
crates/starknet_consensus_orchestrator/src/cende/central_objects.rs
line 370 at r2 (raw file):
Previously, DvirYo-starkware wrote…
document that the
Option<&SierraContractClass>
is only for declare
Done.
crates/starknet_consensus_orchestrator/src/cende/central_objects.rs
line 492 at r2 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
as discussed f2f, from process_transactions, I will call a different function for declare and a different function for the rest , and by that eliminate returning none values.
done.
crates/starknet_consensus_orchestrator/src/cende/mod.rs
line 254 at r1 (raw file):
Previously, DvirYo-starkware wrote…
Yes. We will not do it for now, but it is a possible optimization. Remove your comment
// Dvir, what is this? is it still relevant?
Done.
crates/starknet_consensus_orchestrator/src/cende/mod.rs
line 63 at r2 (raw file):
Previously, DvirYo-starkware wrote…
Consider using raw tuple here
not sure it will be pretty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: 9 of 10 files reviewed, 5 unresolved discussions (waiting on @dafnamatsry)
crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs
line 442 at r3 (raw file):
error!("Failed to prepare blob for next height: {e:?}"); }) .ok();
for what the ok()
?
Code quote:
.ok();
crates/starknet_consensus_orchestrator/src/cende/central_objects_test.rs
line 135 at r3 (raw file):
"central_transaction_execution_info.json"; fn resource_bounds() -> AllResourceBounds {
What is the reason for the change here?
crates/starknet_consensus_orchestrator/src/cende/central_objects_test.rs
line 147 at r3 (raw file):
} fn declare_class_hash() -> ClassHash {
Can we do it as a const
? also in declare_compiled_class_hash
crates/starknet_consensus_orchestrator/src/cende/central_objects_test.rs
line 649 at r3 (raw file):
let expected_sierra_contract_classes = vec![(declare_class_hash(), sierra_contract_class()); 2]; assert_eq!(aerospike_blob.contract_classes, expected_sierra_contract_classes);
Now that we have more complex logic to convert txs to central format I think we should test it.
something like, creating a vector with one tx from each type, convert and check the result. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 9 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dafnamatsry and @Yael-Starkware)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dafnamatsry and @DvirYo-starkware)
crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs
line 442 at r3 (raw file):
Previously, DvirYo-starkware wrote…
for what the
ok()
?
otherwise the compiler is complaining for unused result.
changed it to let _ =
crates/starknet_consensus_orchestrator/src/cende/central_objects_test.rs
line 135 at r3 (raw file):
Previously, DvirYo-starkware wrote…
What is the reason for the change here?
yes, this is the typed that the consensusTransaction uses.
crates/starknet_consensus_orchestrator/src/cende/central_objects_test.rs
line 147 at r3 (raw file):
Previously, DvirYo-starkware wrote…
Can we do it as a
const
? also indeclare_compiled_class_hash
no, because it calls a function, but is can be lazy static. but I don't think it makes it more readable.
crates/starknet_consensus_orchestrator/src/cende/central_objects_test.rs
line 649 at r3 (raw file):
Previously, DvirYo-starkware wrote…
Now that we have more complex logic to convert txs to central format I think we should test it.
something like, creating a vector with one tx from each type, convert and check the result. wdyt?
we already check the conversion of each transaction separately. so I don't see any reason to check the conversion again.
I added a check for the keeping the order and length of the transaction list.
616f6d4
to
89638d4
Compare
Benchmark movements: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @dafnamatsry)
No description provided.